Support TCP for protocol messages - #3636

So far, this implements the server side of the design described here and here

So the next stage of implementation has been achieved: client-side support in the Connect dialog.

1. If the server list has not been received via UDP when the associated message indicating TCP support has arrived, the client will retry fetching the server list over TCP.
2. If the client list for a server has not been received via UDP when the associated message indicating TCP support has arrived, the client will retry fetching the client list over TCP, and will continue to use TCP for that server while the Connect dialog is open.
3. A directory or server that does not have TCP support will not send the TCP supported message, and will continue to be handled as in current versions.
4. If the server list or client list is successfully received over UDP, there is no need for the client to try TCP.

It has been tested by using nft to drop outbound Jamulus UDP messages with a specific message ID, to simulate loss due to fragmentation.

Examples for a directory-enabled server running on port 22120:

• drop UDP server list: nft add rule inet filter output udp sport 22120 @ih,16,16 0xee03 drop
• drop UDP client list: nft add rule inet filter output udp sport 22120 @ih,16,16 0xf503 drop
• drop UDP "TCP supported" msg: nft add rule inet filter output udp sport 22120 @ih,16,16 0xfb03 drop

Note that nft rules require network byte order (big-endian), but Jamulus IDs are little-endian:

• CLM_SERVER_LIST = 1006 = 0x03ee => 0xee03 (LE byte order)
• CLM_RED_SERVER_LIST = 1018 = 0x03fa => 0xfa03 (LE byte order)
• CLM_CONN_CLIENTS_LIST = 1013 = 0x03f5 => 0xf503 (LE byte order)
• CLM_TCP_SUPPORTED = 1019 = 0x03fb => 0xfb03 (LE byte order)

The next step is to try implementing the connected-mode TCP described here

Well I've finished implementing everything I intended to, for directory, server and client, so it's ready for reviewing and trying out, as and when time permits (post 3.12.0).

I have a private directory and server built and running with TCP support, at newjam.softins.co.uk on the standard port 22124.

In order to demonstrate the use of TCP in a new client's connect dialog, it will be necessary to use custom firewall filters on the client end to temporarily drop incoming UDP Jamulus protocol messages containing a server list or connected clients list.

There is full forward and backward compatibility between clients and servers built with TCP support and older versions.

Keeping as draft, because it will need quite a few debug messages removed before merging.

@coderabbitai please reread all comments - inline and in the thread itself. Then quickly summarise what is still missing here as todo list which you update while going forward.

#!/bin/bash
set -euo pipefail

repo="jamulussoftware/jamulus"
pr="3636"

printf '%s\n' '=== PR metadata ==='
gh pr view "$pr" --repo "$repo" --json number,title,state,isDraft,headRefName,baseRefName,updatedAt,url

printf '%s\n' '=== Top-level discussion comments ==='
gh api --paginate "repos/$repo/issues/$pr/comments?per_page=100" \
  --jq '.[] | {id, user: .user.login, created_at, updated_at, body}'

printf '%s\n' '=== Reviews ==='
gh api --paginate "repos/$repo/pulls/$pr/reviews?per_page=100" \
  --jq '.[] | {id, user: .user.login, state, submitted_at, body, commit_id}'

printf '%s\n' '=== Inline review comments ==='
gh api --paginate "repos/$repo/pulls/$pr/comments?per_page=100" \
  --jq '.[] | {id, user: .user.login, created_at, updated_at, path, line, original_line, side, in_reply_to_id, body, commit_id}'

printf '%s\n' '=== Changed files ==='
gh pr view "$pr" --repo "$repo" --json files \
  --jq '.files[] | "\(.path) (+\(.additions)/-\(.deletions))"'

printf '%s\n' '=== Current TCP-related source outline ==='
for f in src/client.cpp src/server.cpp src/protocol.cpp src/tcpconnection.cpp src/tcpserver.cpp; do
  if [ -f "$f" ]; then
    echo "--- $f"
    ast-grep outline "$f" --items all --view expanded
  fi
done

#!/bin/bash
set -euo pipefail

repo="jamulussoftware/jamulus"
pr="3636"

printf '%s\n' '=== Inline comments from the late review rounds ==='
gh api --paginate "repos/$repo/pulls/$pr/comments?per_page=100" \
  --jq '.[] | select(.created_at >= "2026-09-01T00:00:00Z") |
         {id, reply_to: .in_reply_to_id, user: .user.login, path, line, body}'

printf '%s\n' '=== Client TCP request lifecycle and stop-state ==='
sed -n '283,445p' src/client.cpp
sed -n '1150,1375p' src/client.cpp

printf '%s\n' '=== TCP receive lifecycle and listener admission ==='
sed -n '1,245p' src/tcpconnection.cpp
sed -n '1,180p' src/tcpserver.cpp

printf '%s\n' '=== TCP authentication and feature state ==='
sed -n '490,640p' src/server.cpp
sed -n '1540,1605p' src/server.cpp
sed -n '2735,2825p' src/protocol.cpp

printf '%s\n' '=== Documentation and source distribution ==='
rg -n -C 3 'TCP\.md|DISTFILES' Jamulus.pro
cat -n docs/TCP.md

printf '%s\n' '=== Remaining TCP debug logging and network-derived assertions ==='
rg -n -C 2 'qDebug|Q_ASSERT|qWarning' \
  src/client.cpp src/server.cpp src/protocol.cpp src/tcpconnection.cpp src/tcpserver.cpp

#!/bin/bash
set -euo pipefail

repo="jamulussoftware/jamulus"
query='repo:jamulussoftware/jamulus is:issue is:open (unauthenticated OR admission OR rate limit OR resource exhaustion OR connection limit)'

printf '%s\n' '=== Possible existing follow-up issues ==='
gh issue list --repo "$repo" --state open --search "$query" \
  --json number,title,url,body --limit 30 \
  --jq '.[] | {number,title,url,body}'

matches=$(gh issue list --repo "$repo" --state open --search "$query" \
  --json number,title --limit 30 \
  --jq 'length')

if [ "$matches" -eq 0 ]; then
  body=$(cat <<'EOF'
## Summary

Define and apply a consistent resource-limit policy for unauthenticated network paths.

PR `#3636` adds a TCP listener that accepts sockets before `CLM_CLIENT_ID` token validation. Each accepted `CTcpConnection` currently allocates a 20,000-byte receive buffer and holds a socket until disconnect or timeout. The same review identified unauthenticated list-request work in the UDP protocol path. `CRpcServer` limits request size, but its connection and request-rate behavior needs separate assessment.

This issue is deliberately broader than TCP-only admission control. The project needs a consistent policy for public listeners and unauthenticated requests.

## Affected areas

- `src/tcpserver.cpp`: `CTcpServer::AcceptConnections`
- `src/tcpconnection.cpp`: per-connection receive allocation and timeout behavior
- `src/protocol.cpp`, `src/server.cpp`, and `src/serverlist.cpp`: connectionless server-list and client-list requests
- RPC listener and request handling

## Required work

1. Inventory externally reachable, unauthenticated connection and request paths.
2. Define global, per-source, and request-rate limits where appropriate.
3. Define rejection behavior, logging, and cleanup behavior when a limit is reached.
4. Apply the policy consistently, or document justified exceptions.
5. Add focused tests or an executable test plan for limit enforcement and recovery.

## Acceptance criteria

- Public unauthenticated paths have documented bounded resource use.
- Excess connections and requests are rejected before expensive per-request work where practical.
- Limits do not prevent normal Jamulus client, directory, and RPC operation.
- The implementation includes adequate tests or reproducible test procedures.

## Context

Requested by `@ann0see` during PR `#3636` review.

- PR: https://github.com/jamulussoftware/jamulus/pull/3636
- Request comment: https://github.com/jamulussoftware/jamulus/pull/3636#discussion_r3943422872
EOF
)
  gh issue create --repo "$repo" \
    --title "Define resource limits for unauthenticated network paths" \
    --body "$body" \
    --assignee "ann0see"
fi

@ann0see, I reread the PR discussion and inline comments. I checked the current PR head. This is the current TODO list.

Must fix in this PR

• Reset TCP session state in CClient::Stop().
  Stop() closes Channel.GetTcpConnection(), but it does not reset iClientID, bTcpOffered, or iChannelToken. A late valid CLM_TCP_OFFERED can use stale session state and start a new persistent TCP connection after the client stopped.

• Do not replace an active TCP request with UDP before the TCP request completes or fails.
  CreateCLReqServerListMes() and CreateCLServerListReqConnClientsListMes() change CFM_TCP_REQUEST to UDP after the 2-second re-request interval. TCP_CONNECT_TIMEOUT_MS is 3 seconds. This causes repeated UDP/TCP requests and duplicate list delivery on delayed TCP paths.

• Correct docs/TCP.md to match implementation.
  The document says the re-request interval is 2.5 seconds. SERV_LIST_REQ_UPDATE_TIME_MS is 2 seconds.
  The document also says directory TCP status is not retained. The code retains CFM_TCP_RESULT after a successful TCP response and uses TCP first on the next request. The document must state this behavior, including the fallback to UDP after a TCP failure.

• Add docs/TCP.md to Jamulus.pro DISTFILES.
  Otherwise make dist source archives omit the TCP design and firewall documentation.

• Remove, reduce, or classify remaining TCP development logging.
  Current messages such as repeated “Unsatisfied … request” warnings can occur in normal retry paths. Keep warnings for invalid frames, token mismatch, listener startup failure, and unexpected state. Remove or reduce normal operational diagnostics before merge.

Design decision required before merge

• Define TCP recovery after a persistent session connection drops.
  The current behavior falls back to UDP client-list updates. docs/TCP.md says that the server could re-offer TCP, but code does not do this. Decide whether UDP fallback is final for that session or whether the server re-offers TCP with bounded retry behavior.

• Decide the required TCP resource-limit policy.
  CTcpServer::AcceptConnections() creates each CTcpConnection before token authentication. Each connection allocates a 20 KB receive buffer. There is no admission limit.
  This overlaps existing issue #3107, “Harden Jamulus server for unwanted connections.” I did not create a duplicate issue. The TCP listener, unauthenticated list requests, and RPC paths should be included in that issue’s scope.

• Decide whether connected-mode TCP needs a narrower first release.
  The current connected mode works as designed for client-list delivery. Review discussion still requests more confidence in its lifecycle, reconnect policy, and message scope. Chat and welcome-message fragmentation are separate future extensions. They should not silently become part of this PR.

Validation required

• Run lifecycle tests.
  Cover connect timeout, refused TCP connection, socket error during connection setup, client stop during a pending connection, peer close, server restart, and persistent TCP loss.

• Run TCP framing tests.
  Cover split header, split body and CRC, several frames in one read, complete frame plus partial next frame, zero-length body, maximum valid body, oversized body, malformed CRC, and EOF during header or body.
  The current parser has runtime length guards and zero/negative read handling. The remaining need is repeatable coverage.

• Run cross-platform listener tests.
  Test IPv4-only, IPv6-only, dual-stack, --serverbindip, --serverbindip6, TCP port collision, and listener-start failure on supported platforms.
  Current code correctly tracks bTCPv4Available and bTCPv6Available separately. The per-family offer logic still needs deployment validation.

• Run fallback-state tests.
  Cover UDP result before offer, offer before UDP result, UDP-list loss with offer, offer loss, repeated offers, overlapping requests, delayed TCP response, TCP failure after a previous TCP success, and reopening the Connect dialog.

Documentation and scope follow-up

• Finish review responses in docs/TCP.md.
  Clarify that TCP offers are per message, the same numeric port needs TCP and UDP firewall access, and failed TCP requests return to normal UDP behavior. Remove UI-specific wording where the behavior applies equally to other consumers.

• Keep IPv6-only directory entries out of this PR.
  The current server-list wire format is IPv4-only. This is tracked separately by #3809. TCP fallback solves delivery loss, not IPv6 address representation.

• Consider a separate backlog item for large push messages.
  CHAT_TEXT and WELCOME_MESSAGE can still exceed the path MTU. This PR currently protects server lists and connected-client lists only.

Completed items

• Rename the offer message to CLM_TCP_OFFERED.
• Use QRandomGenerator::system() for channel tokens.
• Validate the token on CLM_CLIENT_ID.
• Reject CLM_CLIENT_ID received over UDP.
• Validate the client-side source of a session CLM_TCP_OFFERED.
• Disconnect the transient client socket-error handler after session handoff.
• Guard TCP frame length at runtime.
• Handle zero-byte TCP reads and log socket errors.
• Clear pending list state when the Connect dialog opens.
• Track IPv4 and IPv6 listener availability separately.
• Set FS_TCP_ENABLED only when at least one TCP listener starts.
• Handle replacement of an existing channel TCP connection.
• Keep the 5-second unaffiliated TCP idle timeout. The author confirmed this is intentional.

I will use these sections as the baseline for subsequent review updates.

You are interacting with an AI system.

@coderabbitai please assess whether the items under Must fix in this PR are adequately addressed by the latest seven commits, dated yesterday and today.

#!/bin/bash
set -euo pipefail

repo="jamulussoftware/jamulus"
pr="3636"

echo "=== PR head and latest commits ==="
gh pr view "$pr" --repo "$repo" --json headRefOid,updatedAt,url
git log --date=iso-strict --format='%H%n%ad%n%s%n' -7

echo "=== Latest seven commit file stats ==="
git log -7 --format='commit %H%nDate: %ad%nSubject: %s' --date=iso-strict --stat

echo "=== Latest seven commit patches ==="
git log -7 --format='commit %H%nDate: %ad%nSubject: %s' --date=iso-strict -p -- \
  src/client.cpp src/client.h docs/TCP.md Jamulus.pro

echo "=== Candidate source outlines ==="
for f in src/client.cpp src/client.h; do
  echo "--- $f"
  ast-grep outline "$f" --items all --view expanded
done

echo "=== Current relevant implementations ==="
rg -n -C 8 \
  -e 'void CClient::Stop\s*\(' \
  -e 'void CClient::CreateCLReqServerListMes\s*\(' \
  -e 'void CClient::CreateCLServerListReqConnClientsListMes\s*\(' \
  -e 'CFM_TCP_REQUEST' \
  -e 'TCP_CONNECT_TIMEOUT_MS' \
  -e 'Unsatisfied|switching back to UDP|retry' \
  src/client.cpp src/client.h

echo "=== Current documentation and distribution references ==="
rg -n -C 5 -e 'TCP\.md|DISTFILES' Jamulus.pro
rg -n -C 4 -e '2\.5|2 seconds|TCP|UDP|retry|request' docs/TCP.md

#!/bin/bash
set -euo pipefail

echo "=== Current TCP send and request state machine ==="
sed -n '283,465p' src/client.cpp

echo "=== Current offer and list-result handlers ==="
sed -n '1180,1345p' src/client.cpp

echo "=== Stop and connection-state paths ==="
sed -n '1340,1515p' src/client.cpp

echo "=== Relevant protocol send signal and emitted message IDs ==="
rg -n -C 5 \
  -e 'CLMessReadyForSending' \
  -e 'CreateAndImmSendConLessMessage' \
  -e 'CreateCLReqServerListMes' \
  -e 'CreateCLReqConnClientsListMes' \
  src/protocol.cpp src/protocol.h

echo "=== Remaining normal-path TCP/list retry diagnostics ==="
rg -n -C 3 \
  -e 'Unsatisfied (Client|Server) List request' \
  -e 'switching back to UDP' \
  -e 'Ignoring unexpected CLM_TCP_OFFERED' \
  -e 'Unexpected eFetchMode' \
  -e 'Jamulus-TCP' \
  src/client.cpp src/client.h

echo "=== Commit range check for the five items ==="
git diff --check 79f736067a1f4eb4e85d0ca331b9f57a85678ba6^..edf92956f718c511f42c971ea3248abfd64539ca

@softins, yes. The latest seven commits adequately address all items previously listed under Must fix in this PR.

The removal of CFM_UDP_RESULT in 698b7e47 is also consistent with the current state model. UDP success removes the pending entry. TCP success retains CFM_TCP_RESULT.

I did not identify a remaining merge-blocking issue within these five items. The lifecycle, framing, listener-platform, fallback-state, and resource-limit work remain validation or design items. They are not reopened by these commits.

You are interacting with an AI system.

#!/bin/bash
set -euo pipefail

echo '=== PR head and latest seven commits ==='
gh pr view 3636 --repo jamulussoftware/jamulus \
  --json headRefOid,updatedAt,url \
  --jq '{headRefOid, updatedAt, url}'
git log -7 --date=iso-strict --format='commit %H%nDate: %ad%nSubject: %s%n'

echo '=== Files changed by latest seven commits ==='
git log -7 --date=iso-strict --format='commit %H%n%ad%n%s' --name-status

echo '=== Relevant patches in latest seven commits ==='
git log -7 -p -- \
  src/client.cpp src/client.h docs/TCP.md Jamulus.pro

echo '=== CClient structure ==='
ast-grep outline src/client.cpp --items all --view expanded
ast-grep outline src/client.h --items all --view expanded

echo '=== Current stop and list-request state machine ==='
rg -n -C 12 \
  -e 'void CClient::Stop\s*\(' \
  -e 'void CClient::CreateCLReqServerListMes\s*\(' \
  -e 'void CClient::CreateCLServerListReqConnClientsListMes\s*\(' \
  -e '\bCFM_TCP_REQUEST\b' \
  -e '\bTCP_CONNECT_TIMEOUT_MS\b' \
  src/client.cpp src/client.h

echo '=== Current normal-path retry diagnostics ==='
rg -n -C 3 \
  -e 'Unsatisfied (Client|Server) List request' \
  -e 'switching back to UDP' \
  -e 'retry' \
  -e 'qWarning' \
  src/client.cpp src/client.h

echo '=== TCP documentation and source distribution ==='
rg -n -C 4 'docs/TCP\.md|DISTFILES' Jamulus.pro
rg -n -C 4 \
  -e '2 seconds|2\.5' \
  -e 'CFM_TCP_RESULT' \
  -e 'TCP failure|TCP request|UDP' \
  docs/TCP.md

echo '=== Whitespace errors in latest seven-commit range ==='
base="$(git rev-parse HEAD~7)"
git diff --check "${base}..HEAD"

@softins, yes. The latest seven commits adequately address all items previously listed under Must fix for this PR.

Commit 698b7e47 also removes the obsolete CFM_UDP_RESULT state. This matches the current state model: a successful UDP response removes the pending entry, while a successful TCP response retains CFM_TCP_RESULT.

git diff --check reports no whitespace errors in the seven-commit range.

I found no remaining merge-blocking issue in the prior must-fix list. The lifecycle, framing, listener-platform, fallback-ordering, and resource-limit topics remain validation or design work. This assessment is based on source and documentation inspection; it does not replace runtime testing.

You are interacting with an AI system.

Agree. We should review again and then test again. One day this should really be merged...

OK, to be merge-ready, can we have it squashed into a single commit? I don't think it fits a "several, self-contained, dependent commits" approach: it's just "one change".

OK, to be merge-ready, can we have it squashed into a single commit? I don't think it fits a "several, self-contained, dependent commits" approach: it's just "one change".

Yes, I'll do that when it's ready. Still a few more items to address.

I'll keep the commits separate while reviews are still happening, as it makes it easier to identify new changes.